Skip to content

Add a regression test for host allocators that return null - #162

Open
kadircanyildirm-crypto wants to merge 1 commit into
nasa:mainfrom
kadircanyildirm-crypto:fix/allocator-null-contract
Open

Add a regression test for host allocators that return null#162
kadircanyildirm-crypto wants to merge 1 commit into
nasa:mainfrom
kadircanyildirm-crypto:fix/allocator-null-contract

Conversation

@kadircanyildirm-crypto

@kadircanyildirm-crypto kadircanyildirm-crypto commented Aug 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Rebased onto main. #189 landed the null check this PR proposed, in RustSystemAllocator (crates/spacewasm_util/src/lib.rs:15-20) and in the fuzzing SystemAllocator, so the fix itself is gone from the diff. What is left:

  • crates/spacewasm_util/tests/page_null.rs: a PageAllocator over RustSystemAllocator must report an unsatisfiable page as Err, not serve a page at address zero. Passes on main; with the lib.rs check reverted it fails.
  • tests/util/spectest.rs: the same check in SpecTestAllocator, the one allocator of that shape Clean up code-base from external review #189 did not touch.

Verified on 2eceda3: fmt, clippy (--workspace --all-targets --all-features -D warnings), page_null on x86-64 and i686, also with strict-assertions, and regression_integration. Happy to close instead if you would rather not carry the test.

AI use per AI_POLICY.md: Claude Code assisted with the rebase, the verification runs and this description; scope is crates/spacewasm_util/tests/ and tests/util/, no src/ change.

@kadircanyildirm-crypto
kadircanyildirm-crypto force-pushed the fix/allocator-null-contract branch from ac9edc5 to 0da4074 Compare August 11, 2026 23:20
@kadircanyildirm-crypto

kadircanyildirm-crypto commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main after #163. The conflict was additive: the 64 MiB cap stays exactly as written and the null check goes under it.

#163 added FuzzAllocator::allocate in the same file using NonNull::new(..).ok_or(AllocError::AllocationFailed). This makes SystemAllocator::alloc, just above it, consistent with that.

Re-verified on d384e10b: build, fmt, clippy and the tests pass, and the new test still fails against the unpatched allocator.

@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

CoreMark Benchmark Results

Current Score: 245.539
Baseline Score (main): 233.991
Difference: +11.548 (4.94%)

@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

Code Coverage Report

Current Coverage: 95.30%
Baseline Coverage (main): 95.30%
Difference: +0.00%

@Kronos3 Kronos3 added the Post-v1.0 Issues and features that will not be addressed until after 1.0 Release label Aug 13, 2026
@kadircanyildirm-crypto kadircanyildirm-crypto changed the title Return an allocation error instead of a null pointer from host allocators Add a regression test for host allocators that return null Sep 3, 2026
@kadircanyildirm-crypto

kadircanyildirm-crypto commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto main after #189, which landed the null check itself. The PR now only carries the regression test and the matching check in SpecTestAllocator; details in the updated description.

AI use per AI_POLICY.md: Claude Code assisted with the rebase, the verification runs and this comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Post-v1.0 Issues and features that will not be addressed until after 1.0 Release

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kadircanyildirm-crypto @Kronos3